-
-
Notifications
You must be signed in to change notification settings - Fork 160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add support for Hybrid MBR #168
Conversation
@@ -0,0 +1,62 @@ | |||
{ disks ? [ "/dev/sda" ], ... }: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a suggestion how we should name hybrid.nix
in that case since it's still a bit confusing to have both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I comment here #166 (comment) existent gpt-bios-compat.nix
is enough. hybrid.nix
or hybrid-tmpfs-onroot.nix
looks confusing and redundant for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, the gpt-bios-compat can't do EFI because it has no efi or boot partition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I would also prefer something that can support both EFI/BIOS. We have installer that works with both EFI and BIOS and makes it unnecessary for people to have to figure out what mode there machines was booted from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, so maybe we could reorganize as:
remove gpt-bios-compat.nix
(I'm not sure which is the use case of this without EFI partition)
EDIT: Ok, use case is boot larger disks in BIOS ONLY systems.
remove hybrid-tmpfs-onroot.nix
(or not, but for clarity on discussion)
then if we want to keep the hybrid keyword:
hybrid.nix
-> hybrid_boot.nix
Also, being stricts, seems like the real name of hybrid boot using bios_grub
flags is GPT-only protocol. As in https://wiki.archlinux.org/title/Arch_boot_process#Feature_comparison and https://repo.or.cz/syslinux.git/blob/HEAD:/doc/gpt.txt.
or refered as "BIOS/GPT boot" in wikipedia: https://en.wikipedia.org/wiki/BIOS_boot_partition
hybrid.nix
-> bios-gpt-boot.nix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, maybe can we get this to a conclusion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can call it bios-efi-boot? since it does both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we should support this, seems kinda clunky and I don't see many usecases? can't this be achieved with the existing table type and some extra flags?
{ | ||
type = "hybrid_partition"; | ||
gptPartitionNumber = 1; | ||
mbrPartitionType = "0x0c"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are other valid mbrPartitionType options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whatever hexcode dos table partitions support, as in https://en.wikipedia.org/wiki/Partition_type#List_of_partition_IDs.
If null
then it guess partition type based on GPT one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but would it still be a hybrid mbr if the partitionType is not "0x0c"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, set to 0x0c
have sense for RPi because RPi boot process look exactly for it, but other use cases could work with default value.
hybrid_partitions = [ | ||
{ | ||
type = "hybrid_partition"; | ||
gptPartitionNumber = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't the index in the list enough? why do we need to count here? what would happen if I set this to 3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to choose which GPT partition want to make available in DOS table. Maybe a sane default using index is ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this is the index of the gpt partition table which is defined in content?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct.
Sure not. Use cases are multiple, mine is to create a working SD Card for Rapsberry Pi. But is also used frequently to boot multiple operating systems setups. In anycase the proposed interfaces makes very explicit the use of this feature, and also clean and flexible enough for people craft their disks partitioning without affecting layers below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the structure of the hybrid_table
type should be simlified.
- remove the additional
hybrid_partitions
list and instead introduce a new partition type containing all fields - this will get rid of the
gptPartitionNumber
, as there is only one list of partitions left
Modified example:
{ disks ? [ "/dev/sda" ], ... }:
{
disk = {
main = {
type = "disk";
device = builtins.elemAt disks 0;
content = {
type = "hybrid_table";
efiGptPartitionFirst = false;
partitions = [
{
# the `type` field is not needed and can be omitted
type = "hybrid_partition";
name = "TOW-BOOT-FI";
start = "0%";
end = "32MiB";
fs-type = "fat32";
content = {
type = "filesystem";
format = "vfat";
mountpoint = null;
};
# mbr specific fields
mbrPartitionType = "0x0c";
bootable = true; # previously named `mbrBootableFlag`
}
{
type = "partition";
name = "ESP";
start = "32MiB";
end = "512MiB";
bootable = true;
content = {
type = "filesystem";
format = "vfat";
mountpoint = "/boot";
};
}
# ...
];
};
};
};
}
internal = true; | ||
description = "Hybrid MBR/GPT Partition table"; | ||
}; | ||
efiGptPartitionFirst = lib.mkOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this really need to be configurable? What is the use case? Why not always place the GPT partition first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GPT partition first is always recommended for GRUB bootloader, but, for instance, Raspberry Pi bootloader, looks exactly for a 0x0c
partition type as first partition.
IMHO, disko may support every flow gdisk
hybrid tool support. As related in "Creating a Hybrid MBR" from https://www.rodsbooks.com/gdisk/hybrid.html
Please add a test for the new module, like for example: { pkgs ? (import <nixpkgs> { })
, makeDiskoTest ? (pkgs.callPackage ./lib.nix { }).makeDiskoTest
}:
makeDiskoTest {
name = "boot-hybrid";
disko-config = ../example/hybrid-mbr.nix;
extraTestScript = ''
machine.succeed("mountpoint /boot");
'';
} |
Replaced by #508 |
Closes #166